OCPBUGS-85519: Add Agent IRI credentials to global pull secret - #6067
OCPBUGS-85519: Add Agent IRI credentials to global pull secret#6067sadasu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@sadasu: This pull request references Jira Issue OCPBUGS-85519, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExports IRI credential helpers; InternalReleaseImage controller now syncs IRI credentials into the configured global pull secret and refactors event/filter logic; template controller no longer merges at render time; tests updated to expect creds already present in the global pull secret. ChangesIRI Credentials Migration to InternalReleaseImage Controller
Sequence Diagram(s) sequenceDiagram
participant InternalReleaseImageController
participant KubeAPI
participant GlobalPullSecret
participant IRIAuthSecret
InternalReleaseImageController->>KubeAPI: Get ControllerConfig (resolve pullSecret ref)
InternalReleaseImageController->>KubeAPI: Get GlobalPullSecret
InternalReleaseImageController->>KubeAPI: Get IRIAuthSecret
InternalReleaseImageController->>InternalReleaseImageController: ExtractIRICredentials(IRIAuthSecret, ControllerConfig)
InternalReleaseImageController->>InternalReleaseImageController: MergeIRIRegistryCredentialsIntoPullSecret(GlobalPullSecret.data, password, baseDomain)
InternalReleaseImageController->>KubeAPI: Update GlobalPullSecret (if changed)
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
eba73aa to
21b8d9e
Compare
|
@sadasu: This pull request references Jira Issue OCPBUGS-85519, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 414-417: The reconcile only calls syncGlobalPullSecret but the
controller doesn't requeue on all resources that can stale the global pull
secret; update the controller's watches (SetupWithManager / Setup) so Reconcile
is triggered when the cluster pull secret (the Kubernetes Secret used as the
global pull secret) is created/updated/deleted, when ControllerConfig changes
(spec.pullSecret and spec.dns.baseDomain), and when any resource that can rotate
credentials (e.g., DockerRegistryKey) changes in addition to the existing
IRI-owned secrets. Concretely: add a watch for the cluster/global pull Secret
(by name/namespace or label) to enqueue affected InternalReleaseImage objects,
add a watch on ControllerConfig and filter to requeue when Spec.PullSecret or
Spec.DNS.BaseDomain changes, and ensure DockerRegistryKey and the two IRI-owned
secret watches remain; keep syncGlobalPullSecret as-is and rely on these
additional watches to ensure it runs whenever the global pull secret can become
stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a3af203-7768-4e8d-b9bc-30a62398657d
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
21b8d9e to
a2a957e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/template/template_controller.go`:
- Around line 584-586: The Secret filter currently hard-codes the name
"pull-secret" so changes to a differently named global pull secret won't
re-enqueue the template controller; update filterSecret to compare the incoming
Secret's name against the configured value from ControllerConfig.Spec.PullSecret
(accessed via the controller's config instance, e.g., c.config.Spec.PullSecret
or controllerConfig variable) and namespace as appropriate, falling back to
"pull-secret" only if Spec.PullSecret is empty; ensure you reference the
controller's config in filterSecret (filterSecret,
ControllerConfig.Spec.PullSecret) so the watch reacts to the actual configured
secret name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eed7f0d9-2394-4efe-ac5d-d3d662b75431
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
|
/retest-required |
a2a957e to
3efb3e1
Compare
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 318-324: The deleteSecret handler does a direct type assertion on
the incoming obj which can be a cache.DeletedFinalStateUnknown tombstone and
will panic; update deleteSecret to detect if obj is a
cache.DeletedFinalStateUnknown, extract the actual Secret from tombstone.Obj (or
fall back when the cast fails), then proceed to call ctrl.filterSecret(secret)
and ctrl.queue.Add(ctrlcommon.InternalReleaseImageInstanceName) as before;
reference the deleteSecret function and reuse the existing filterSecret method
and ctrl.queue.Add call so behavior matches other delete handlers like
deleteInternalReleaseImage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82ffd110-9096-496f-a2e8-260a8d8c61f5
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/template/template_controller_test.go
- pkg/controller/common/iri_secret_merger.go
3efb3e1 to
2c82ee0
Compare
openshift-ci
Bot
removed
the
needs-rebase
2c82ee0 to
28bb4db
Compare
|
/retest-required |
|
/verified with help from @bfournie. The global pull secret on both the master and worker had contained the IRI credentials. |
|
@sadasu: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @bfournie. The global pull secret on both the master and worker contain the IRI credentials after this fix. |
openshift-ci-robot
added
the
verified
|
@sadasu: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/test ? |
|
/test e2e-agent-compact-ipv4-iso-no-registry Better to get a green run on this job for this PR |
|
|
||
| if oldCfg.Spec.Images[templatectrl.DockerRegistryKey] == curCfg.Spec.Images[templatectrl.DockerRegistryKey] { | ||
| // Check if any fields relevant to syncGlobalPullSecret have changed | ||
| dockerRegistryChanged := oldCfg.Spec.Images[templatectrl.DockerRegistryKey] != curCfg.Spec.Images[templatectrl.DockerRegistryKey] |
There was a problem hiding this comment.
nit: (not blocking) this looks like a little bit too over-engineered. I do not expect a lot of updates on the (singleton) controller config resource (a simpler but less complicated code here could work as well very likely)
| } | ||
|
|
||
| // Check if this is the configured global pull secret | ||
| cfg, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) |
There was a problem hiding this comment.
Is there really a chance to have a different name for the global pull-secret? If not I'd prefer to remove this code and use a simpler check by name
| } | ||
|
|
||
| // Sync IRI credentials into the global pull secret | ||
| if err := ctrl.syncGlobalPullSecret(cconfig, iriRegistryCredentialsSecret); err != nil { |
There was a problem hiding this comment.
I think this design has been already discarded previously (see this thread) due the regeneration.
To avoid that @rwsu implemented the current approach (modify directly /var/lib/kubelet/config.json) which does not work as we discussed.
So the merge should happen directly in the point of the code where the global pull-secret is rendered (template controller)
28bb4db to
569dc69
Compare
openshift-ci-robot
removed
the
verified
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@sadasu: This pull request references Jira Issue OCPBUGS-85519, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/test unit |
569dc69 to
31ca624
Compare
|
@sadasu it looks like there are some conflicts to be resolved (v1 version could be directly used) |
openshift-ci
Bot
added
the
needs-rebase
| if err != nil { | ||
| return ctrl.syncFailingStatus(cfg, fmt.Errorf("could not merge IRI registry credentials into pull secret: %w", err)) | ||
| // Sync IRI credentials into the global pull secret if IRI is enabled | ||
| if err := ctrl.syncGlobalPullSecret(cfg); err != nil { |
There was a problem hiding this comment.
I see two potential points here:
-
ctrl.syncGlobalPullSecretcan potentially update the global pullsecret with the IRI credentials (at least the first time), but theclusterPullSecretRawremains stale in such case, since it's not updated. This means that later the MC are rendered with a stale pullsecret. The previous update will trigger subsequently another sync, and another re-rendering of the MCs (this time with the correct pullsecret), which was what we wanted to avoid.
So this seems a relatively easy thing to fix to avoid an unnecessary re-rendering. -
Any kind of error in
syncGlobalPullSecretis managed as a failure, and stops the current sync. To avoid blocking the MC rendering maybe it'd be better to just log the issue
With these two fixes I think we could reach the desired behavior previously discussed, avoiding unneeded MC renderings
There was a problem hiding this comment.
Valid points. Fixing.
31ca624 to
84433e7
Compare
openshift-ci
Bot
removed
the
needs-rebase
84433e7 to
79d0f15
Compare
openshift-ci
Bot
added
the
needs-rebase
79d0f15 to
96d0510
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
96d0510 to
77ac699
Compare
|
/pipeline required |
|
Scheduling tests matching the |
The IRI credentials were being added to /var/lib/kubelet/config.json which gave only kubelet the necessary credentials to pull images from the IRI registry. Fix now adds the IRI credentials to the global pull secret so that other components like the MCO could also pull images from this internal registry.
77ac699 to
96dfdad
Compare
|
/test bootstrap-unit |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
The IRI credentials were being added to /var/lib/kubelet/config.json which gave only kubelet the necessary credentials to pull images from the IRI registry.
Fix now adds the IRI credentials to the global pull secret so that other components like the MCO could also pull images from this internal registry.
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit